Skip to content

fix(tools): keep ix stdout when the command exits non-zero - #20

Merged
KageBinary merged 2 commits into
mainfrom
fix/539-saferun-keeps-stdout
Sep 1, 2026
Merged

fix(tools): keep ix stdout when the command exits non-zero#20
KageBinary merged 2 commits into
mainfrom
fix/539-saferun-keeps-stdout

Conversation

@KageBinary

Copy link
Copy Markdown
Collaborator

Step 1 of the sequence ix-infrastructure/Ix#539 lays out: the plugins must tolerate a non-zero exit before the CLI starts producing one. That issue names this plugin explicitly as a reason ix locate cannot simply be fixed.

Two commits — the second is what #539 asks for; the first is a bug I hit while testing it and could not leave.

1. fix(runtime): abort timers leaked on the failure path

callRuntime and getRuntime cleared their abort timer only after a successful fetch. When the runtime is unreachable the fetch rejects, so the timer stayed pending for its full 5s — and a pending timer keeps the event loop alive, so the host process hung for five seconds at exit on every call. isRuntimeAvailable never captured its timer at all, leaking on every path including success.

This is the normal case, not an edge case: these tools have a CLI fallback precisely because most machines do not run the Core Runtime.

Measured against an unreachable runtime — the call itself returns in ~20ms, so only the exit is delayed:

before after
child process exit 5010 ms 28 ms

Found because it made the tests below time out at bun's 5s default.

2. fix(tools): keep stdout when ix exits non-zero

Bun's $ throws on a non-zero exit and .text() discards stdout with it, so safeRun returned null for any failed command — and ix-docs-tool fell back to a generic "Not found in graph", discarding the diagnostics ix had supplied.

.nothrow() keeps stdout. It returns "" both for a missing binary and for a failure with no output, and both still map to null, so the ix-unavailable path is unchanged.

The tests run in a child process, deliberately

Bun's shell resolves binaries from the real process PATH. Neither mutating process.env.PATH nor $.env({PATH}) nor .env({PATH}) redirects it — I verified all three. An in-process stub is silently ignored, and the test then runs the developer's real ix against their real graph (which is exactly what happened on my first attempt). A child process with its own PATH is the only way to stub it, and it covers the real spawn path as a bonus.

Verification

Each new test was checked to fail without its fix, not just pass with it:

  • with the timer fix in place but safeRun reverted, keeps the JSON body when ix exits non-zero fails on its assertion in 54ms — showing exactly the #539 symptom rather than a timeout.
  • reverting both fails all four.

Full suite 95 passed / 0 failed (91 existing + 4 new), 649ms.

`callRuntime` and `getRuntime` cleared their abort timer only after a
successful fetch. When the runtime is unreachable the fetch rejects, so the
timer stayed pending for its full 5s -- and a pending timer keeps the event
loop alive, so the host process hung for five seconds at exit on every call.

That is the normal case, not an edge case: the tools have a CLI fallback
precisely because most machines do not run the Core Runtime, and every one of
those calls paid the five seconds.

`isRuntimeAvailable` never captured its timer at all, so it leaked
HEALTH_TIMEOUT_MS on every path including success.

Move all three into `finally`. Measured against an unreachable runtime: the
call returns in ~20ms and the process now exits at 28ms instead of 5010ms.

Invisible in-process -- only the exit is delayed -- so the regression test
measures how long a child takes to exit after the call resolves.
Bun's `$` throws on a non-zero exit and `.text()` discards stdout along with
it, so `safeRun` returned null for any `ix` command that failed. Several `ix`
commands already exit 1 to mean "you asked for something that does not exist"
while still printing a useful JSON body, and `locate` is about to join them
(Ix#539) -- at which point ix-docs-tool would lose the diagnostics ix supplied
and fall back to a generic "Not found in graph".

`.nothrow()` keeps stdout. It returns "" both for a missing binary and for a
failure with no output, and both still map to null, so the ix-unavailable path
is unchanged.

This is step 1 of Ix#539's sequence: the plugins must tolerate a non-zero exit
before the CLI starts producing one.

The tests run in a child process. Bun's shell resolves binaries from the real
process PATH -- neither mutating `process.env.PATH` nor `$.env({PATH})` nor
`.env({PATH})` redirects it, so an in-process stub is silently ignored and the
test would run the developer's real `ix` against their real graph. A child with
its own PATH is the only way to stub it, and it covers the real spawn path.

Refs ix-infrastructure/Ix#539
@KageBinary
KageBinary merged commit 833c515 into main Sep 1, 2026
6 checks passed
KageBinary added a commit that referenced this pull request Sep 1, 2026
#20 landed first and appended its own `NonZeroExitDiagnostics` block to the
end of `tests/tools.test.ts`; this branch appends `LiveIxCli` to the same
place. Git saw one conflict region covering both additions.

They are independent -- `LiveIxCli` is opt-in behind `IX_LIVE_TESTS` and drives
the tools against a real `ix`, while `NonZeroExitDiagnostics` runs a stubbed
`ix` in a child process -- so both are kept, and both sets of imports survived
the merge.

bun test: 119 pass, 3 skip, 0 fail.
KageBinary added a commit that referenced this pull request Sep 1, 2026
#20 fixed this for `ix-docs-tool.ts`. It is the only tool it fixed, and this
repo has **no shared runner** -- all 17 tools call bun's `$` directly, and not
one of the other 16 used `.nothrow()`. So the repo was still, in the ways that
matter, exactly as broken as before that PR.

Bun's `$` throws on a non-zero exit and `.text()` discards stdout along with
it. Several `ix` commands already exit 1 to mean "you asked for something that
does not exist" while printing a complete JSON body, and Ix#547 takes that from
three commands to thirteen. Every one of those bodies was being thrown away and
rendered as "ix unavailable" -- a wrong answer, and a far less useful one than
the record ix supplied.

## The distinction now preserved

    exit 1 with a body   -> ix answered. The body IS the answer.
    exit 1 with no body  -> ix could not answer. Report it.
    could not run at all -> ix is not installed. Report it.

`runtime/cli.ts` is the shared runner that draws it: `runIx` returns
`{stdout, stderr, exitCode}` and is null only when the command could not run,
`safeRun` is the stdout-or-null shape most callers want, and `failureDetail`
turns a failure into a message that prefers ix's own stderr. `safeRun` moves
here from `ix-docs-tool.ts` rather than being reimplemented, so there is one
definition.

`tryLlm` in `runtime/llm.ts` had the same hole and, since #15, is the *primary*
path -- a body dropped there fell through to the JSON path, which dropped it a
second time.

## Deliberately left alone

Five call sites, because for them a throw IS the signal:

  - `ix --version` in `ix-health.ts` (x2) and `detectVersion` in `runtime/llm.ts`
    -- capability probes; `--version` exits 0 whenever ix works at all.
  - `command -v ix` in `ix-ingest.ts` -- presence check.
  - `ix map` in `ix-ingest.ts` (x2) -- a write. A failed map is a failure, and
    the existing catch already reports why.

## Tests

Six cases, per tool rather than per helper, because a shared runner is only
half the fix and a tool that never adopted it is still broken. Same
child-process harness #20 introduced, and for the same reason: bun's `$`
resolves from the real process PATH, so an in-process stub silently runs the
developer's real `ix`.

Five of the six fail with the source reverted and the tests kept
(mutation-checked, twice). The sixth -- an empty non-zero exit still reporting
unavailable -- passes either way by design: it is the guard that this change
did not over-correct into treating every failure as an answer.

bun test: 125 pass, 3 skip, 0 fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant